Refactor framerate definitions and resolution entries for D40x removing 5 FPS - #380
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the framerate and resolution definitions for the D40x RealSense device family by introducing helper macros for cleaner code structure and removing 5 FPS support across all D40x sensors. The changes consolidate resolution entries using new macros and reorganize framerate array definitions for better maintainability.
Changes:
- Removed 5 FPS support from all D40x resolution/framerate combinations
- Introduced DS5_RES and D401_COMMON_RES helper macros to reduce code duplication
- Added dedicated D40x RGB sensor format configuration (ds5_40x_rgb_format) with new resolution definitions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DS5_RES(848, 480, ds5_framerate_15_60)\ | ||
| DS5_RES(640, 480, ds5_framerate_15_60)\ |
There was a problem hiding this comment.
The resolutions 848x480 and 640x480 now support framerates {15, 30, 60} (ds5_framerate_15_60) instead of the previous {5, 15, 30, 60, 90} (ds5_framerate_to_90). This removes not only 5 FPS as mentioned in the PR description, but also removes 90 FPS support for these resolutions. The loss of 90 FPS support should be verified as intentional, as it's not mentioned in the PR description which only states "removing 5 FPS".
| DS5_RES(848, 480, ds5_framerate_15_60)\ | |
| DS5_RES(640, 480, ds5_framerate_15_60)\ | |
| DS5_RES(848, 480, ds5_framerate_15_90)\ | |
| DS5_RES(640, 480, ds5_framerate_15_90)\ |
| .n_framerates = ARRAY_SIZE(ds5_framerate_90), | ||
| }, | ||
| D401_COMMON_RES | ||
| DS5_RES(256, 144, ds5_framerate_15_90) |
There was a problem hiding this comment.
The resolution 256x144 previously only supported 90 FPS (ds5_framerate_90), but now supports {15, 30, 60, 90} (ds5_framerate_15_90). This is actually an expansion of functionality rather than just the removal of 5 FPS as stated in the PR description. While this may be intentional and beneficial, it represents an undocumented feature addition.
| static const struct ds5_resolution d40x_rgb_sizes[] = { | ||
| DS5_RES(1280, 800, ds5_framerate_15_30) | ||
| D401_COMMON_RES | ||
| }; |
There was a problem hiding this comment.
The newly created d40x_rgb_sizes array defines resolutions and framerates for the D40X RGB sensor. Previously, the D40X device would have fallen through to the default case in the switch statement and used ds5_onsemi_rgb_sizes, which has completely different resolutions (640x480, 960x720, 1280x720, 1920x1080, 2048x1536). This new configuration uses {1280x800, 1280x720, 848x480, 640x480, 640x360, 480x270, 424x240} instead. This is a major functional change that affects the RGB sensor capabilities and should be verified against the actual D40X hardware specifications.
| DS5_RES(640, 480, ds5_framerate_15_60)\ | ||
| DS5_RES(640, 360, ds5_framerate_15_90)\ | ||
| DS5_RES(480, 270, ds5_framerate_15_90)\ | ||
| DS5_RES(424, 240, ds5_framerate_15_90)\ |
There was a problem hiding this comment.
The macro D401_COMMON_RES ends with a backslash continuation character on line 706 that continues to an empty line 707. While this is valid C, the trailing backslash is unnecessary and could be confusing. The macro should end without a trailing backslash after the last DS5_RES() invocation, as is standard practice for multi-line macro definitions.
| DS5_RES(424, 240, ds5_framerate_15_90)\ | |
| DS5_RES(424, 240, ds5_framerate_15_90) |
5306721 to
f4dc0c4
Compare
Refactor framerate definitions for D40x by removing the 5 FPS entry and consolidating resolution entries for better clarity and maintainability. This change enhances the structure of the framerate and resolution definitions.